fix(player): report and fail closed on runtime delivery errors - #3472

Reviewed the diff (all 9 files) plus the unmodified context around the changed methods (_deliverRuntimeData, _trySetRuntimeDataDirect, _replayRuntimeData, _runtimeBridgeReady gating) to check whether "fail closed" actually holds end-to-end. Solid direction — the structuredClone guard, the applied/error event pair, and the opaque-sandbox option are all good primitives. Found a few real gaps and one design inconsistency worth addressing before/after merge.

1. _sendControl still fails open when contentWindow is null
packages/player/src/hyperframes-player.ts (_sendControl, ~L552-578):

this.iframe.contentWindow?.postMessage(...)
return true;

If contentWindow is null at call time, the optional chaining short-circuits the whole postMessage call — nothing throws, execution falls through to return true, and for set-runtime-data/clear-runtime-data no runtimedataerror fires. That's exactly the silent-drop this PR is trying to close everywhere else. _runtimeBridgeReady narrows the window (it's only true after a ready message), but it doesn't eliminate it — e.g. iframe torn down/mid-navigation between ready and the next attribute-driven reset, or any host embedding scenario where contentWindow transiently goes null without isConnected flipping false in the same tick. Suggest treating contentWindow == null as a failure for these two actions (return false / dispatch runtimedataerror) rather than treating it as a no-op success.

2. No timeout — a hung or never-delivered message reports neither success nor failure
The whole applied/error signal pair depends on the iframe's runtime actually running and posting back (runtime-data-applied / runtime-data-error in runtimeData.ts → init.ts → runtime-message-handler.ts). There's no timeout anywhere in this stack. If the postMessage is delivered but the iframe hasn't registered its message listener yet (classic race, already called out in the _replayBridgeState doc comment for other control messages), if the handler promise never settles, or if the iframe crashes/unloads mid-flight, the caller gets silence forever — not an error, not an applied event. For an API whose stated goal is "fail closed on runtime delivery errors," an indefinite hang is arguably the worst failure mode since it's indistinguishable from "still pending." Worth at least documenting the guarantee explicitly (fire-and-forget beyond N ms is unconfirmed), or adding a timeout that emits runtimedataerror if neither signal arrives within a bound.

3. Two different failure-reporting mechanisms for the same conceptual error
setRuntimeData() throws synchronously when structuredClone is unavailable or the payload isn't cloneable, but postMessage-delivery failures and handler failures (sync throw, async rejection) are reported asynchronously via runtimedataerror CustomEvent. A caller that wraps player.setRuntimeData(...) in try/catch (the natural reflex given it's documented to throw) will only catch the clone-failure case and will believe delivery succeeded even when it silently failed downstream unless they've also wired up addEventListener("runtimedataerror", ...). Consider documenting this split explicitly in the README (both failure channels, not just the throw), since it's easy to get partial coverage.

4. runtimedataapplied/runtimedataerror carry no correlation id — concurrent updates on the same channel are ambiguous
deliver() in runtimeData.ts fires reportApplied(channel) / reportError(channel, error) keyed only by channel name. If setRuntimeData("captions", A) is followed quickly by setRuntimeData("captions", B) before the first async handler invocation resolves, both in-flight handler calls eventually resolve/reject independently, and both report against the same channel string with no way for a listener to tell which payload the applied/error event refers to. A slow/superseded resolution for A arriving after B was already applied would look like a fresh confirmation of the latest state. The new test (runtimeData.test.ts) only exercises sequential calls (await vi.waitFor(...) between the two setRuntimeData calls), so this ordering case isn't covered. If rapid same-channel updates are a realistic use case (captions certainly sounds like one), consider a monotonic sequence number or generation token in the message payload so late-resolving stale attempts can be ignored/identified by the caller.

5. Sandbox-origin: unrecognized attribute values silently fall back to the more permissive mode
_applySandboxOriginPolicy():

if (policy === "opaque") { this.iframe.sandbox.remove("allow-same-origin"); return; }
this.iframe.sandbox.add("allow-same-origin");

Any value other than exactly "opaque" (including a typo like "Opaque" or "opaqu") falls through to the same-origin-allowed branch — i.e., the less isolated default. For a security-relevant toggle in a PR themed around failing closed, an unrecognized value silently choosing the less restrictive posture is the wrong default direction. Not high severity since the attribute is developer-set and not attacker-controlled in the common case, but worth a defensive console.warn or treating unrecognized non-null values as opaque instead of same-origin.

Nit: _sendControl's catch branch only special-cases action === "set-runtime-data" || action === "clear-runtime-data" for event dispatch; other control actions (play, seek, set-volume, etc.) keep the pre-existing fully-silent failure behavior. That's consistent with the PR's stated scope (runtime-data delivery specifically), just flagging so it's not read as "all control messages now fail closed" — they don't, only these two.

Nice test additions overall (structuredClone-unavailable throw, postMessage DataCloneError → runtimedataerror, sandbox attribute toggling, async apply/reject reporting) — the coverage gap is specifically the concurrent-update-ordering and null-contentWindow cases above, which aren't hit by the current suite.

Additive exact-head review at 0dced883. Miga already covers the null-contentWindow, no-timeout, fail-open unknown sandbox value, mixed failure channels, and uncorrelated concurrent-update gaps. Two concrete consequences make this a request-changes verdict.

The structured-clone guard and async handler rejection reporting are good primitives, and the full current check set is green.

blocker — dynamic sandbox-origin changes do not change the active document's sandbox. _applySandboxOriginPolicy() only adds/removes the allow-same-origin token (hyperframes-player.ts:281-289), and the new test changes the attribute after the connected player has loaded while checking only the DOMTokenList. HTML sandbox flags take effect when the iframe navigates; changing/removing tokens has no effect on the already-loaded document. A caller switching a live same-origin player to opaque therefore sees the attribute/test say isolated while the composition retains parent-DOM access. Reload/recreate the iframe on policy change, or make the policy immutable/pre-navigation-only, and prove the origin-access boundary in a real-browser test. Unrecognized non-null values should resolve to the restrictive policy, not same-origin.

blocker — a superseded async attach can emit a false applied event. runtimeData.ts:14-21 reports runtime-data-applied whenever the handler promise fulfills, with only the channel attached. The caption consumer's hfCV2Attach intentionally fulfills without applying when its sequence is stale (if (seq !== hfCV2Attach.seq) return). Under rapid A→B updates, stale A can therefore emit “captions applied” after B even though A was discarded, and the host cannot distinguish it. Add a per-channel generation/request token and suppress or identify stale completions; pin out-of-order resolution with a concurrent test.

The null-window and unbounded-no-response paths Miga identified remain independently blocking for a fail-closed delivery contract; I am not duplicating their full write-up here.

— Magi

Verdict: REQUEST CHANGES
Reasoning: The new signals can claim isolation/application that did not actually occur, so the API does not yet fail closed at its security or async completion boundaries.

Re-reviewed at 8a7ddcb. Diffed against the previous review's 5 findings, plus checked CI.

1. _sendControl fails open on null contentWindow — RESOLVED
_sendControl now captures contentWindow once, and on !frameWindow for set-runtime-data/clear-runtime-data it calls _rejectRuntimeDataDelivery(..., "Composition iframe is unavailable") and returns false instead of silently no-oping. Same treatment in the catch branch (postMessage throw, e.g. DataCloneError) — both paths now fire runtimedataerror. Covered by hyperframes-player.test.ts: "reports a null iframe window as a delivery failure" and "reports postMessage delivery failures instead of silently dropping runtime data."

2. No timeout in the applied/error signal chain — RESOLVED
_beginRuntimeDataDelivery arms a 10s window.setTimeout per channel that rejects with "Runtime data delivery timed out after 10000ms" if neither runtime-data-applied nor runtime-data-error arrives. Pending deliveries are also proactively rejected on disconnect (disconnectedCallback), src/srcdoc navigation, and sandbox-origin reload (_rejectAllRuntimeDataDeliveries), so a torn-down iframe doesn't just wait for the timeout to fire. Covered by "reports a bounded error when the runtime never responds" (fake timers).

3. Split error-reporting design (sync throw vs. async CustomEvent) — PARTIALLY ADDRESSED
Structurally unchanged: invalid channel and non-cloneable payload (structuredClone throwing, or now-missing structuredClone support) still throw synchronously from setRuntimeData; delivery/application failures after that point still report async via runtimedataerror. What's new is documentation — the README now explicitly calls out "Invalid channels and non-cloneable payloads throw synchronously. Failures after the call returns are reported with runtimedataerror..." and tells callers to listen for both. That mitigates the "easy to miss a path" risk but doesn't unify the two error channels into one contract. I'd call this closed-enough for this PR (it's a design tradeoff, now at least documented) rather than a blocking gap.

4. No correlation ID — RESOLVED
requestId is threaded end-to-end: player generates a monotonic id in _beginRuntimeDataDelivery, passes it through _trySetRuntimeDataDirect/_sendControl → bridge.ts control handler → runtimeData.ts (resolveRequestId honors the caller-supplied id) → back out via runtime-data-applied/runtime-data-error → _takeRuntimeDataDelivery matches on (channel, requestId) before resolving/rejecting, so stale completions from a superseded request are dropped. runtimeData.ts additionally tracks a generation per channel so an in-flight async handler promise from an older setRuntimeData call can't fire reportApplied/reportError after a newer call has superseded it. Covered by "reports only the latest concurrent delivery on a channel" (runtimeData.test.ts) and "ignores a superseded completion and correlates the latest application" (hyperframes-player.test.ts).

5. _applySandboxOriginPolicy treats unrecognized values as permissive — RESOLVED
Switched from a value-specific check to hasAttribute(SANDBOX_ORIGIN_ATTR) — any non-null value (including a typo like "opaqu") now removes allow-same-origin, i.e. fail-closed. Test "treats every non-null sandbox-origin value as restrictive" confirms. This PR also closes the reload gap Via mentioned: attributeChangedCallback now triggers _reloadForSandboxOriginPolicy() when the attribute value actually changes on a connected element, since sandbox flags only take effect on navigation — otherwise toggling the attribute would silently do nothing to a live iframe. There's a real browser-driven test for this (tests/browser/sandbox-origin.ts, wired into player-perf.yml on the load shard) that actually loads a fixture in a real sandboxed iframe and asserts window.parent access is blocked/allowed across default → opaque → typo'd-opaque → removed, rather than just asserting DOMTokenList contents. Good — that's the kind of verification a unit test alone can't give you here (jsdom doesn't enforce sandbox), so I'm reasonably confident this actually works, not just "looks right."

Summary: 4/5 resolved, 1/5 mitigated via documentation rather than structurally fixed (acceptable).

CI: Typecheck is red — must be fixed before merge.

packages/player/src/runtime-in-srcdoc.ts(47,36): error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.

This is in the new findOpeningTag helper added by the "refactor(player): simplify runtime tag scanner" commit:

const boundary = lower[index + prefix.length];
if (OPENING_TAG_BOUNDARIES.has(boundary)) {

lower[...] indexing is typed string | undefined under the stricter scripts/tsconfig.json (noUncheckedIndexedAccess), but OPENING_TAG_BOUNDARIES is Set<string>, so .has(boundary) doesn't type-check there even though it presumably passes under the player package's own (looser) tsconfig. Fix is small — e.g. OPENING_TAG_BOUNDARIES.has(boundary as string) is not it; better to widen the set to Set<string | undefined>, or guard with boundary !== undefined && OPENING_TAG_BOUNDARIES.has(boundary). This needs to be fixed and the Typecheck job green before merge.

Re-reviewed at head 5922da3c. Diffed exactly against the last fully-reviewed commit (8a7ddcbb) — the only change since then is a single line in packages/player/src/runtime-in-srcdoc.ts.

1. Typecheck fix — CORRECT

-const OPENING_TAG_BOUNDARIES = new Set([">", " ", "\t", "\n", "\r", "\f"]);
+const OPENING_TAG_BOUNDARIES = new Set<string | undefined>([">", " ", "\t", "\n", "\r", "\f"]);

lower[index + prefix.length] is string | undefined under noUncheckedIndexedAccess (the stricter scripts/tsconfig.json), so widening the Set's element type is the correct, minimal fix — no as cast, no runtime branch added. Semantically it's also sound: the set's members are still only real boundary characters, so .has(undefined) still evaluates to false when the tag name runs off the end of the string (e.g. a truncated <head with no trailing >), which is exactly the "not a valid boundary, keep scanning / eventually return null" behavior the original /<head[^>]*>/i regex had (it also requires a literal > to match). No behavior change, just makes the existing logic type-check. Typecheck is green in CI at this head.

2. New issues since last review — none. The commit-to-commit diff from 8a7ddcbb → 5922da3c is a 1-line, 1-file change (confirmed via gh api .../compare/8a7ddcbb...5922da3c). Nothing else moved, so there's nothing new to flag in hyperframes-player.ts, runtimeData.ts, bridge.ts, or the sandbox-origin path.

3. Prior 5 findings — still resolved. Since none of that code was touched again after the last review confirmed 4/5 structurally resolved (null contentWindow, timeout, correlation ID via requestId/generation, sandbox-origin fail-closed + reload-on-change with a real-browser boundary test) and 1/5 mitigated via README documentation (split sync-throw vs. async runtimedataerror reporting — accepted as a documented design tradeoff, not a blocker), nothing in this commit could have regressed them.

CI is fully green across the board (Typecheck, Test, Build, Lint, CodeQL, the new Verify sandbox origin boundary (load shard only) browser job, all regression shards).

Verdict: looks good to me — no outstanding blockers from my side.

— Miga

Re-review at exact 5922da3c; my stale change request is closed.

• hyperframes-player.ts:299-319 applies restrictive sandboxing for every non-null policy and reloads the active src/srcdoc; tests/browser/sandbox-origin.ts:12-49 proves default, opaque, typoed-non-null, and removal against real parent-origin access.
• runtimeData.ts:22-50 binds each delivery to channel generation + request ID and suppresses stale handler completions. The player matches (channel, requestId), rejects null windows and postMessage errors, bounds no-response at 10 seconds, and drains pending deliveries on navigation, sandbox reload, and disconnect.
• runtime-in-srcdoc.ts:35-54 keeps the linear tag scanner behavior while widening the boundary set type so indexed undefined remains a non-member; this is the sole delta after Miga's full fix-head pass.

Audited: player delivery lifecycle, runtime data generation/reporting, bridge/message correlation, sandbox reload policy and browser proof, final scanner type fix.

Trusting: unchanged ancillary README/workflow wiring and the fully green current CI/test matrix.

Verdict: APPROVE
Reasoning: isolation and application are now proven/correlated outcomes with bounded failure paths, and the final type-only delta preserves scanner semantics. — Magi